Emmet - Wrap with abbreviation with live preview #45092
Conversation
…microsoft#43345)" This reverts commit 7420a04.
Currently it's only working for single cursor.
- Removed flickering when typing abbreviation - Removed tabstops - Fixed bug when wrapping multiline text
- Added checks for not reverting previews more times than needed, that was causing extra text to be deleted. - Fixed issue when wrapping nodes with multiple level of indentation. - Removed all the undo commands. Now all the logic of going back to the original state is handled by revertPreview.
Refactoring some of the code, now a single object contains the current and original ranges, as well as the original content to wrap
| @@ -24,6 +24,12 @@ interface ExpandAbbreviationInput { | |||
| filter?: string; | |||
| } | |||
|
|
|||
| interface RangesAndContent { | |||
| currentRange: vscode.Range; | |||
There was a problem hiding this comment.
The currentRange here will only have the preview ranges correct? If so, let's rename it to previewRange to make it more readable.
Also rename content to originalContent to match with the originalRange and RangesAndContent to PreviewRangesWithContent
| } | ||
|
|
||
| // On inputBox closing | ||
| return abbreviationPromise.then(inputAbbreviation => { |
There was a problem hiding this comment.
The tests are currently failing as the promise returned by makeChanges is not being returned by wrapWithAbbreviation
Since makeChanges does a revertPreview when given abbreivation is null/undefined/invalid, can we not simplify this to return abbreviationPromise.then(inputAbbreviation => makeChanges(inputAbbreviation, previewMade, true)); ?
| if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) { return false; } | ||
| function makeChanges(inputAbbreviation: string | undefined, previewMade?: boolean, definitive?: boolean): Thenable<any> { | ||
| if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) { | ||
| let returnPromise: Thenable<any> = Promise.resolve(); |
There was a problem hiding this comment.
You can condense these 5 lines to return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();
| returnPromise = revertPreview(editor, rangesToReplace).then(() => { return false; }); | ||
| } | ||
| return returnPromise; | ||
| } | ||
|
|
||
| let extractedResults = helper.extractAbbreviationFromText(inputAbbreviation); |
There was a problem hiding this comment.
This might be an exisitng bug... but shouldnt we check for the validity of the abbreviation after it is extracted instead of before?
Also, if extractedResults is empty then shouldnt we revert the previous preview?
I would suggest to move the extraction to be the first thing that we do in makeChanges(). Then,
if (!extractedResults || !helper.isAbbreviationValid(syntax, extractedResults.abbreviation)) {
return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve();
}
| } | ||
| rangesToReplace.forEach(rangesAndContent => { |
There was a problem hiding this comment.
The currentRange of each rangesToReplace array gets updated inside revertPreview. Since the originalRange doesnt change, it is safe to loop through and use it here before revertPromise is completed. But for the sake of readability, it is better to do this after `revertPromise is completed.
| rangesToReplace.forEach(rangesAndContent => { | ||
| let rangeToReplace = rangesAndContent.originalRange; | ||
| let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n']; | ||
| expandAbbrList.push({ syntax: syntax || '', abbreviation, rangeToReplace, textToWrap, filter }); |
There was a problem hiding this comment.
why would syntax be null/undefined here?
| let textToWrap = rangeToReplace.isSingleLine ? ['$TM_SELECTED_TEXT'] : ['\n\t$TM_SELECTED_TEXT\n']; | ||
| expandAbbrList.push({ syntax, abbreviation, rangeToReplace, textToWrap, filter }); | ||
| return Promise.resolve().then(() => { | ||
| return applyPreview(editor, expandAbbrList, rangesToReplace).then(ranges => { |
There was a problem hiding this comment.
Why return the ranges here from applyPreview and the update rangesToReplace?
rangesToReplace is accessible from inside applyPreview and can be updated from there as well.
Just like you did in revertPreview
| @@ -90,45 +89,34 @@ export function wrapWithAbbreviation(args: any) { | |||
|
|
|||
| function makeChanges(inputAbbreviation: string | undefined, previewMade?: boolean, definitive?: boolean): Thenable<any> { | |||
| if (!inputAbbreviation || !inputAbbreviation.trim() || !helper.isAbbreviationValid(syntax, inputAbbreviation)) { | |||
| return previewMade ? revertPreview(editor, rangesToReplace).then(() => { return false; }) : Promise.resolve(); | |||
| return previewMade ? revertPreview(editor, rangesToReplace) : Promise.resolve(); | |||
There was a problem hiding this comment.
Returning false allows us to not revert the preview several times. That would not a problem per se, since revertPreview only replaces the current range with the original content, but why make extra unnecessary edits?
| function inputChanged(value: string): string { | ||
| if (value !== currentValue) { | ||
| currentValue = value; | ||
| makeChanges(value, previewMade, false).then((out) => { |
There was a problem hiding this comment.
What if inputChanged gets called before the previous makeChanges completes?
| let indentPrefix = ''; | ||
| let preceedingText = editor.document.getText(new vscode.Range(new vscode.Position(thisRange.start.line, 0), thisRange.start)); | ||
| // If there is only whitespace before the text to wrap, take that as prefix. If not, take as much whitespace as there is before text appears. | ||
| if (!preceedingText.match(/[^\s]/)) { |
There was a problem hiding this comment.
Instead of this if/else, why not follow the same logic as preceedingWhiteSpace in https://github.com/Microsoft/vscode/blob/master/extensions/emmet/src/abbreviationActions.ts#L66-L68?
| } | ||
|
|
||
| let newText = expandedText || ''; | ||
| newText = newText.replace(/\n/g, '\n' + indentPrefix).replace(/\$\{[\d]*\}/g, '|'); |
There was a problem hiding this comment.
This will not replace placeholders. Try wrapping with !
| indentPrefix = (preceedingText.match(/(^[\s]*)/) || ['', ''])[1]; | ||
| } | ||
|
|
||
| let newText = expandedText || ''; |
There was a problem hiding this comment.
Why would we continue if expandedText is null/undefined/empty?
… preceeding whitespace, added a check for validity of expandedtext on each selection.
Adding ability to have a preview of the resulting text when calling the Wrap with Abbreviation command, as the user types in the input box.
Feature request here #23169